Add option to use MS1Quantity for quantifications for Spectronaut#117
Merged
tonywu1999 merged 6 commits intodevelfrom Feb 10, 2026
Merged
Add option to use MS1Quantity for quantifications for Spectronaut#117tonywu1999 merged 6 commits intodevelfrom
tonywu1999 merged 6 commits intodevelfrom
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces hard-coded F handling with an intensity_column_mapping resolved at runtime, exposes three intensity options (PeakArea, NormalizedPeakArea, MS1Quantity) in docs and function defaults, updates callers/renames to use resolved column, adds a unit test, and removes an earlier intensity type-check. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@R/clean_Spectronaut.R`:
- Around line 21-26: The code does not validate the intensity argument before
mapping, so intensity_column_mapping[[intensity]] can return NULL and silently
drop the intensity column; update the validation in the function that defines
intensity_column_mapping by ensuring intensity is one of the allowed keys (e.g.
use match.arg(intensity, choices = names(intensity_column_mapping)) or if (!
intensity %in% names(intensity_column_mapping)) stop(...)), then perform the
lookup for intensity_column; additionally, after computing intensity_column
check for NULL and raise a clear error mentioning the accepted options
(referencing intensity_column_mapping and intensity_column).
🧹 Nitpick comments (1)
inst/tinytest/test_clean_Spectronaut.R (1)
1-11: Good coverage of the happy path for MS1Quantity.The test correctly validates that the MS1Quantity column value flows through to
Intensityin the output.Two suggestions:
- Consider adding a negative test for an invalid
intensityvalue to ensure a clear error is raised (especially relevant if the validation fix fromclean_Spectronaut.Ris applied).- Consider adding a test that verifies
PeakAreastill works as expected (regression guard).,
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Add option to use MS1Quantity for quantifications for Spectronaut. Right now, this is not possible.
Changes
Testing
Added unit tests for clean function.
Checklist Before Requesting a Review
Summary by CodeRabbit
New Features
Documentation
Tests
Chores